Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cli: improve secure preset #818

Merged
merged 7 commits into from
Nov 2, 2021
Merged

cli: improve secure preset #818

merged 7 commits into from
Nov 2, 2021

Conversation

david-yu
Copy link
Contributor

@david-yu david-yu commented Oct 29, 2021 •
edited
Loading

Changes proposed in this PR:

  • Enable gossipEncryption generation and autoEncrypt in the secure preset.
  • Remove server.bootstrapExpect from demo preset since it defaults to server.replicas as defined in our docs
  • Add controller.enabled to true to demo preset.

How I've tested this PR:

❯ consul-k8s install -preset=secure

==> Pre-Install Checks
No existing installations found.
 ✓ No previous persistent volume claims found
 ✓ No previous secrets found

==> Consul Installation Summary
    Installation name: consul
    Namespace: consul
    Overrides:
    connectInject:
      enabled: true
    controller:
      enabled: true
    global:
      acls:
        manageSystemACLs: true
      gossipEncryption:
        autoGenerate: true
      name: consul
      tls:
        enableAutoEncrypt: true
        enabled: true
    server:
      replicas: 1

    Proceed with installation? (y/N) y

==> Running Installation
 ✓ Downloaded charts
 --> creating 1 resource(s)
 --> Starting delete for "consul-gossip-encryption-autogenerate" PodSecurityPolicy
 --> podsecuritypolicies.policy "consul-gossip-encryption-autogenerate" not found
 --> creating 1 resource(s)
 --> Starting delete for "consul-gossip-encryption-autogenerate" ServiceAccount
 --> serviceaccounts "consul-gossip-encryption-autogenerate" not found
 --> creating 1 resource(s)
 --> Starting delete for "consul-gossip-encryption-autogenerate" Role
 --> roles.rbac.authorization.k8s.io "consul-gossip-encryption-autogenerate" not found
 --> creating 1 resource(s)
 --> Starting delete for "consul-gossip-encryption-autogenerate" RoleBinding
 --> rolebindings.rbac.authorization.k8s.io "consul-gossip-encryption-autogenerate" not found
 --> creating 1 resource(s)
 --> Starting delete for "consul-tls-init" ServiceAccount
 --> serviceaccounts "consul-tls-init" not found
 --> creating 1 resource(s)
 --> Starting delete for "consul-tls-init" Role
 --> roles.rbac.authorization.k8s.io "consul-tls-init" not found
 --> creating 1 resource(s)
 --> Starting delete for "consul-tls-init" RoleBinding
 --> rolebindings.rbac.authorization.k8s.io "consul-tls-init" not found
 --> creating 1 resource(s)
 --> Starting delete for "consul-gossip-encryption-autogenerate" Job
 --> jobs.batch "consul-gossip-encryption-autogenerate" not found
 --> creating 1 resource(s)
 --> Watching for changes to Job consul-gossip-encryption-autogenerate with timeout of 10m0s
 --> Add/Modify event for consul-gossip-encryption-autogenerate: ADDED
 --> consul-gossip-encryption-autogenerate: Jobs active: 0, jobs failed: 0, jobs succeeded: 0
 --> Add/Modify event for consul-gossip-encryption-autogenerate: MODIFIED
 --> consul-gossip-encryption-autogenerate: Jobs active: 1, jobs failed: 0, jobs succeeded: 0
 --> Add/Modify event for consul-gossip-encryption-autogenerate: MODIFIED
 --> consul-gossip-encryption-autogenerate: Jobs active: 1, jobs failed: 0, jobs succeeded: 0
 --> Add/Modify event for consul-gossip-encryption-autogenerate: MODIFIED
 --> Starting delete for "consul-tls-init" Job
 --> jobs.batch "consul-tls-init" not found
 --> creating 1 resource(s)
 --> Watching for changes to Job consul-tls-init with timeout of 10m0s
 --> Add/Modify event for consul-tls-init: ADDED
 --> consul-tls-init: Jobs active: 0, jobs failed: 0, jobs succeeded: 0
 --> Add/Modify event for consul-tls-init: MODIFIED
 --> consul-tls-init: Jobs active: 1, jobs failed: 0, jobs succeeded: 0
 --> Add/Modify event for consul-tls-init: MODIFIED
 --> Starting delete for "consul-gossip-encryption-autogenerate" Job
 --> Starting delete for "consul-tls-init" Job
 --> creating 56 resource(s)
 --> beginning wait for 56 resources with timeout of 10m0s
 --> creating 1 resource(s)
 --> Watching for changes to Job consul-server-acl-init-cleanup with timeout of 10m0s
 --> Add/Modify event for consul-server-acl-init-cleanup: ADDED
 --> consul-server-acl-init-cleanup: Jobs active: 1, jobs failed: 0, jobs succeeded: 0
 --> Add/Modify event for consul-server-acl-init-cleanup: MODIFIED
 --> Starting delete for "consul-server-acl-init-cleanup" Job
 ✓ Consul installed into namespace "consul"

~/projects/consul-k8s/cli david-yu-cli-secure-preset 1m 49s
❯ k get pods -n consul
NAME                                                          READY   STATUS    RESTARTS   AGE
consul-4258h                                                  1/1     Running   0          66s
consul-connect-injector-webhook-deployment-668cd47898-56zjg   1/1     Running   0          66s
consul-connect-injector-webhook-deployment-668cd47898-p9lpj   1/1     Running   0          65s
consul-controller-76bf96646d-58lkh                            1/1     Running   0          66s
consul-ptqfw                                                  1/1     Running   0          66s
consul-q2tbp                                                  1/1     Running   0          66s
consul-server-0                                               1/1     Running   0          64s
consul-webhook-cert-manager-6d8cf874cc-xlh7c                  1/1     Running   0          66s

❯ k exec -it consul-server-0 -n consul -- /bin/sh
/ $ echo $GOSSIP_KEY
fdZ10GVbUjCntfZDipXBot+mxLzAMssfopf0+kPRY4c=

How I expect reviewers to test this PR:

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

ishustava
ishustava approved these changes Oct 29, 2021
View reviewed changes
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just a couple of comments to remove values that are the same as the defaults.

cli/cmd/install/presets.go Outdated Show resolved Hide resolved
cli/cmd/install/presets.go Outdated Show resolved Hide resolved
cli/cmd/install/presets.go Outdated Show resolved Hide resolved
David Yu and others added 3 commits October 29, 2021 10:06
@ishustava
Update cli/cmd/install/presets.go
533aca0 
Co-authored-by: Iryna Shustava <[email protected]>
@ishustava
Update cli/cmd/install/presets.go
b1d85cb 
Co-authored-by: Iryna Shustava <[email protected]>
@ishustava
Update cli/cmd/install/presets.go
f384acc 
Co-authored-by: Iryna Shustava <[email protected]>
ndhanushkodi
ndhanushkodi approved these changes Nov 1, 2021
View reviewed changes
Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great David! Do you think we should include controller=enabled in the demo preset if we're including it in secure for consistency? And should we include the metrics values in secure? The reason I ask is I was imagining secure to be like demo with some secure defaults enabled on top of that.

@david-yu
Copy link
Contributor Author

david-yu commented Nov 2, 2021

Good point about controller enabled to true on the demo preset. As far as the metrics in secure, I believe we currently allow the ability to scrape metrics over TLS which is why I didn't have that in there, otherwise it makes sense to include that.

@david-yu david-yu merged commit 08396a5 into main Nov 2, 2021
@david-yu david-yu deleted the david-yu-cli-secure-preset branch November 2, 2021 07:16
ottaviohartman pushed a commit to ottaviohartman/consul-k8s that referenced this pull request Nov 3, 2021
@lkysow
Acceptance: wait for previous installation to exit (hashicorp#818)
21d92f5 
* Acceptance: wait for previous installation to exit

In some cases the previous installation is listed as uninstalled by Helm
but its pods are still terminating. This can cause issues for the newly
running tests when they query Kubernetes to get information about the
new installation, e.g. getting the server pod IPs might return the IPs
from the old installation as well.

* Make static-client/server exit faster
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ishustava ishustava ishustava approved these changes

@ndhanushkodi ndhanushkodi ndhanushkodi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@david-yu @ndhanushkodi @ishustava